[doc] Usecases with docbase as param #1502

felixmulder · 2016-09-08T18:30:06Z

Cherry-picked the commit from @odersky's branch on inline that refactors Attachment.Key[A] => Property.Key[A] and implemented the docbase as a property - which will then cause the typechecking of usecases only when the docbase property is present in the context.

Other than that, not much has changed from #1472

Review: @odersky

odersky

The logic looks good, but I would like to see better decoupling between core compiler and dottydoc.

odersky · 2016-09-16T07:02:49Z

src/dotty/tools/dotc/core/Comments.scala

+
+    class ExpansionLimitExceeded(str: String) extends Exception
+  }
+}


Why put this in core instead of dottydoc? A lot of the logic seems to be doc-specific. Proposal: Keep Comment as a pure data class as it was in Scanners. Implement the new logic as methods external to Comment. Comment contains only defs (the only val in it could as well be a def), so this is easy.

@odersky: I think the basis of this review rests on the thoughts in this comment, as such let me try to address these concerns here in summary.

My reasons for keeping this in the compiler is for tooling - I think that's the same reason as to why this implementation was in nsc and not in Scaladoc for scalac.

When we have a presentation compiler - we're going to want expanded (or "cooked") comments for the standard library entities.

The Comment in core as such - does not deal with things that dottydoc does - like @group and other dottydoc specific annotations - it only deals with expansion of definitions and the comment inheritance.

odersky · 2016-09-16T07:04:36Z

src/dotty/tools/dotc/core/Contexts.scala

@@ -68,6 +69,9 @@ object Contexts {
    /** The context base at the root */
    val base: ContextBase

+    /** Documentation base */
+    def getDocbase = property(DocContext)


Can we move this out of Context? The design should be ideally one of a plugin. That is, there is a context property, but the Contexts object knows nothing about it. I would do it more like we model defn: Put in the main Doc object:

val DocBase = ... def getDocBase(implicit ctx: Context) = ctx.property(DocBase)

odersky · 2016-09-16T07:05:45Z

src/dotty/tools/dotc/core/Contexts.scala

@@ -573,10 +578,15 @@ object Contexts {
    }
  }

+  val DocContext = new Key[DocBase]
  class DocBase {


DocContext should be renamed to DocBase for consistency.
Both kinds of DocBase should also move to the dottydoc package, I think.

odersky · 2016-09-16T07:06:36Z

src/dotty/tools/dotc/core/SymDenotations.scala

@@ -1515,7 +1515,9 @@ object SymDenotations {

    /** Enter a symbol in given `scope` without potentially replacing the old copy. */
    def enterNoReplace(sym: Symbol, scope: MutableScope)(implicit ctx: Context): Unit = {
-      require((sym.denot.flagsUNSAFE is Private) ||  !(this is Frozen) || (scope ne this.unforcedDecls) || sym.hasAnnotation(defn.ScalaStaticAnnot))
+      def isUsecase = ctx.property(DocContext).isDefined && sym.name.show.takeRight(4) == "$doc"


Move this line to a "isUserCase" method in the dottydoc package.

odersky · 2016-09-16T07:39:05Z

src/dotty/tools/dotc/typer/Typer.scala

@@ -1456,6 +1462,45 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit
    traverse(stats)
  }

+  private def typedUsecases(syms: List[Symbol], owner: Symbol)(implicit ctx: Context): Unit =


I think expandParentDocs belongs in DottyDoc, not in Typer. So I would move typedUseCases there as well. Pass the typer as a paremeter, so it can call back to enterSymbol, createSymbol, typedStats.

odersky · 2016-09-16T07:42:12Z

src/dotty/tools/dotc/util/CommentParsing.scala

-import scala.reflect.internal.Chars._
-
-object CommentUtils {
+object CommentParsing {


Needs a comment what it does.

Also what's the motivation for keeping this in util? Is it used outside dottydoc as well?

odersky · 2016-09-22T09:09:06Z

src/dotty/tools/dotc/core/Comments.scala

+      doc.map(d => _docstrings += (sym -> d))
+  }
+
+  abstract case class Comment(pos: Position, raw: String)(implicit ctx: Context) { self =>


Passing a context into a class always poses the risk of space leaks. It's better not to make the context implicit, so that we can track precisely where it is used. It's even better not to pass a context into a class at all.

odersky · 2016-10-06T12:48:54Z

LGTM!

Also eliminates the need for comment processing to be part of the `DocASTPhase`, so this should be put into a DocMiniPhase

Fixes scala#1502 by making the length field use 1 or 2 bytes, depending on the number of parameters in a signature.

Fix #1544: Allow long signatures in names

felixmulder added In progress and removed In progress labels Sep 8, 2016

felixmulder mentioned this pull request Sep 8, 2016

[doc] Add support for usecases #1472

Closed

11 tasks

felixmulder assigned odersky Sep 8, 2016

odersky reviewed Sep 16, 2016

View reviewed changes

felixmulder force-pushed the topic/dottydoc-usecases-param branch from e4e19e0 to b3d2b2c Compare September 21, 2016 13:02

odersky reviewed Sep 22, 2016

View reviewed changes

felixmulder force-pushed the topic/dottydoc-usecases-param branch from b3d2b2c to 86c9209 Compare October 5, 2016 08:06

dottybot added the reviewed label Oct 6, 2016

felixmulder added 14 commits October 6, 2016 17:06

Add symbols to Doc AST, needed for @usecase

77dc769

Also eliminates the need for comment processing to be part of the `DocASTPhase`, so this should be put into a DocMiniPhase

Add Comments object instead of Scanners.Comment case class

30d8135

Typecheck usecases

567798a

Add UsecasePhase to dottydoc

ade2306

Fix name clashes because of @usecase

1e61c8c

Fix type parameters not getting properly typed

7a52440

Move typing of usecases to typedClassDef

ea24c55

Fix cooking of docstrings

5463b7a

Move docstring parser to dottydoc miniphase

8be7177

Move docstring cooking to dotty

49b1993

Typecheck usecases in fresh local scope

af27562

Implement docbase as property

208232a

Separate ContextDocstrings from Context and make it pluggable

9253616

Remove Context reference in Comment and UseCase

9bcf282

felixmulder force-pushed the topic/dottydoc-usecases-param branch from 86c9209 to 9bcf282 Compare October 6, 2016 16:26

felixmulder merged commit 0426fe7 into scala:master Oct 6, 2016

felixmulder removed the reviewed label Oct 6, 2016

odersky added a commit to dotty-staging/dotty that referenced this pull request Oct 13, 2016

Fix scala#1502: Allow long signatures in names

78fc228

Fixes scala#1502 by making the length field use 1 or 2 bytes, depending on the number of parameters in a signature.

odersky mentioned this pull request Oct 13, 2016

Fix #1544: Allow long signatures in names #1588

Merged

odersky added a commit to dotty-staging/dotty that referenced this pull request Oct 13, 2016

Fix scala#1502: Allow long signatures in names

c04caec

Fixes scala#1502 by making the length field use 1 or 2 bytes, depending on the number of parameters in a signature.

odersky referenced this pull request Oct 16, 2016

Merge pull request #1588 from dotty-staging/fix-#1502

009398b

Fix #1544: Allow long signatures in names

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[doc] Usecases with docbase as param #1502

[doc] Usecases with docbase as param #1502

Uh oh!

felixmulder commented Sep 8, 2016 •

edited

Loading

Uh oh!

odersky left a comment

Uh oh!

odersky Sep 16, 2016 •

edited

Loading

Uh oh!

felixmulder Sep 16, 2016

Uh oh!

odersky Sep 16, 2016 •

edited

Loading

Uh oh!

odersky Sep 16, 2016 •

edited

Loading

Uh oh!

odersky Sep 16, 2016 •

edited

Loading

Uh oh!

odersky Sep 16, 2016

Uh oh!

odersky Sep 16, 2016

Uh oh!

odersky Sep 22, 2016

Uh oh!

odersky commented Oct 6, 2016

Uh oh!

Uh oh!

[doc] Usecases with docbase as param #1502

[doc] Usecases with docbase as param #1502

Uh oh!

Conversation

felixmulder commented Sep 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odersky left a comment

Choose a reason for hiding this comment

Uh oh!

odersky Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixmulder Sep 16, 2016

Choose a reason for hiding this comment

Uh oh!

odersky Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky Sep 16, 2016

Choose a reason for hiding this comment

Uh oh!

odersky Sep 16, 2016

Choose a reason for hiding this comment

Uh oh!

odersky Sep 22, 2016

Choose a reason for hiding this comment

Uh oh!

odersky commented Oct 6, 2016

Uh oh!

Uh oh!

felixmulder commented Sep 8, 2016 •

edited

Loading

odersky Sep 16, 2016 •

edited

Loading

odersky Sep 16, 2016 •

edited

Loading

odersky Sep 16, 2016 •

edited

Loading

odersky Sep 16, 2016 •

edited

Loading